Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed a bug caused by paths with spaces in Windows. #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixed a bug caused by paths with spaces in Windows. #51

wants to merge 2 commits into from

Conversation

nicber
Copy link

@nicber nicber commented Oct 1, 2016

If I specify
g:clang_format='"C:/Program Files (x86)/LLVM/bin/clang-format.exe"' (For example)
then the call to executable() fails because the executable function gets confused by the quotes, which
are necessary to escape the spaces.
This patch removes those quotes before testing if it is executable.

rhysd
rhysd requested changes Oct 2, 2016
View reviewed changes
autoload/clang_format.vim
" but executable() get confused by the quotes, so we remove them
if has("win32") && !executable(split(g:clang_format#command, '"')[0])
return 1
elseif !has("win32") && !executable(g:clang_format#command)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Conditions of if are complicated. I think it's good to separate them to multiple if statements
  • Indentation looks collapsed
if has('win32') || has('win64')
    if !executable(split(...))
        return 1
    endif
elseif !executable(g:clang_format#command)
    return 1
endif

Copy link
Author

@nicber nicber Oct 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed these in the latest commit.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@rhysd
Copy link
Owner

rhysd commented Oct 2, 2016

I think quoting command name is not good. Does escaping white spaces such as some\ dir work well?

@nicber
Copy link
Author

nicber commented Oct 2, 2016

Unfortunatelly, escaping whitespace with \ does not work.

@rhysd
Copy link
Owner

rhysd commented Oct 2, 2016

I noticed that this is a problem when the command is run. I'll commit a fix for it tomorrow. Could you try it before merging this PR?

@nicber
Copy link
Author

nicber commented Oct 2, 2016

Sure, I'll test it.
My clang-command is '"C:/Program Files (x86)/LLVM/bin/clang-format.exe"'
I investigated this issue and found the following:

  • Quoting the path is necessary for system() to work.
  • Quoting makes the call to executable() fail.
  • If I don't quote the path, the failures are inversed.
    So, the solution I found was to remove the quotes before calling executable.

@rhysd
Copy link
Owner

rhysd commented Oct 3, 2016

@nicber

Please try latest with below config (removing quotes)

let g:clang_format#command = 'C:/Program Files (x86)/LLVM/bin/clang-format.exe'

@nicber
Copy link
Author

nicber commented Oct 3, 2016 •
edited
Loading 

Error detected while processing function clang_format#replace[2]..<SNR>45_verify_command[1]..clang_format#is_invalid:
line   10:
E684: list index out of range: 0
E15: Invalid expression: v[0] < 3 || (v[0] == 3 && v[1] < 4)
Error detected while processing function clang_format#replace[42]..<SNR>45_error_message:
line    1:
clang-format has failed to format.

I get this error with the latest commit.

@rhysd
Copy link
Owner

rhysd commented Oct 4, 2016

Thank you for the report. It looks to resolve the quoting problem on Windows 😄 And it seems to fail to check clang version. I'll fix version check more strict. Could you show the output of clang-format.exe --version?

rhysd added a commit that referenced this pull request Oct 5, 2016
@rhysd
give up checking version when failing to obtain the version
20f7c94 
related: #51
@rhysd
Copy link
Owner

rhysd commented Oct 5, 2016 •
edited
Loading

Could you try the latest? (and please let me know the output of clang-format --version to fix skipping version check)

@nicber
Copy link
Author

nicber commented Oct 5, 2016

Sorry for the delay
I get this error with latest

Error detected while processing function clang_format#replace[42]..<SNR>45_error_message:
line    1:
clang-format has failed to format.

This is the result of clang-format --version

C:\Users\nicol>"C:/Program Files (x86)/LLVM/bin/clang-format.exe" --version
clang-format version 3.9.0 (branches/release_39)

@rhysd
Copy link
Owner

rhysd commented Oct 5, 2016 •
edited
Loading

Thank you for confirmation. Hmm... I think clang-format command failed by some reason. I'll investigate it on my Windows machine (I actually don't have Windows machine for development...)

Please ensure that your code can be formatted correctly from command line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhysd rhysd rhysd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nicber @rhysd